Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates theme_json_* filter names with ending *_data #3443

Closed
wants to merge 3 commits into from

Conversation

kebbet
Copy link

@kebbet kebbet commented Oct 11, 2022

Adding _data to filter names.
https://core.trac.wordpress.org/ticket/56796

@@ -180,7 +180,7 @@ public static function get_core_data() {
*
* @param WP_Theme_JSON_Data Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'theme_json_default', new WP_Theme_JSON_Data( $config, 'default' ) );
$theme_json = apply_filters( 'theme_json_default_data', new WP_Theme_JSON_Data( $config, 'default' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea might be to follow a theme_json_data_{$context} syntax:

Suggested change
$theme_json = apply_filters( 'theme_json_default_data', new WP_Theme_JSON_Data( $config, 'default' ) );
$theme_json = apply_filters( 'theme_json_data_default', new WP_Theme_JSON_Data( $config, 'default' ) );

@@ -490,7 +490,7 @@ public static function get_user_data() {
*
* @param WP_Theme_JSON_Data Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'theme_json_user', new WP_Theme_JSON_Data( $config, 'custom' ) );
$theme_json = apply_filters( 'theme_json_user_data', new WP_Theme_JSON_Data( $config, 'custom' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SergeyBiryukov
Copy link
Member

Should we consider putting data in the middle, e.g. theme_json_data_default, etc. to closer match WP_Theme_JSON_Data?

Not sure about adding the wp_ prefix, but seems to make sense here too? theme_json_get_style_nodes would then also need the prefix.

@hellofromtonya
Copy link
Contributor

Test failures are unrelated to this PR and are currently present in trunk.

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Ready for commit

@hellofromtonya
Copy link
Contributor

Committed via changeset https://core.trac.wordpress.org/changeset/54501. Thank you everyone for your contributions 🎉

@oandregal
Copy link
Member

oandregal commented Oct 13, 2022

This PR needs a few follow-ups:

  • The devnote for 6.1 (it had already been published when this PR was proposed). I've gone ahead and update it.
  • The documentation for this in the handbook.
  • The same code in Gutenberg.

I've prepared WordPress/gutenberg#44940 to address the last two items. Would anyone be available for review?

@@ -1899,7 +1899,7 @@ protected static function get_style_nodes( $theme_json, $selectors = array() ) {
*
* @param array $nodes Style nodes with metadata.
*/
return apply_filters( 'theme_json_get_style_nodes', $nodes );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scruffian @ajlende @draganescu I see that, in Gutenberg land, we name this filter gutenberg_theme_json_style_nodes. Do we need to take any action here? I haven't seen that we prefix the filters depending on environment (wp_ or gutenberg_) in any other case. My understanding is that it's problematic for any consumer that want to use the filter. Though this rationale may have changed, so I've excluded this from WordPress/gutenberg#44940 and can be looked at separately, if necessary.

Copy link

@ajlende ajlende Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging this @oandregal. I don't see any specific reason for using a different filter in the plugin vs core. I'll open a PR to match this.

EDIT: Opened WordPress/gutenberg#44949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants